…ore (sec)

Follow-up to #543. Two contract regressions discovered in code review:

H1: deleteAll() silently skipped pending/orphan rows because it now
    delegates to deleteById (added in #543) which short-circuits when
    pending=true. Result: no public API cleared orphan markers; a
    "wipe scope" call silently leaked the very rows operators need
    to clean up. Adds forceDeleteById that bypasses the pending-skip;
    deleteAll uses it. deleteById (single-key) unchanged.

H2: get/has/delete now threw TypeError for invalid keys because they
    all routed through vaultId(). ICredentialStore documents these as
    returning undefined/false on missing keys; throwing breaks
    substitutability with InMemoryCredentialStore / EncryptedKvCredentialStore.
    Adds isSafeKey() predicate; readers short-circuit; put() still
    throws via vaultId (only path that persists a colliding id).

M1: SAFE_SEGMENT widened from [A-Za-z0-9_-] to [A-Za-z0-9._:-] so
    natural credential names like "openai.prod" and "scope:billing"
    are accepted. Still rejects path separators and whitespace.

5 new tests + 1 rewritten test. Existing tests pass.

…ore (sec) (#544)

* fix(storage): harden ServerCredentialStore (key injection, delete races, orphan reasons)

Three classes of issue addressed:

1. Key-injection scope escape.
   userId, projectId, and every credential key are now matched against a
   strict SAFE_SEGMENT grammar (^[A-Za-z0-9_-]{1,128}$). Without this, a key
   like "../../other-user/other-project/leaked" could be smuggled through
   vaultId() and collide with another scope's vault id. Failures are surfaced
   as TypeError without echoing the invalid value back.

2. Delete races and orphan-overwrite.
   - delete() is refactored onto an internal deleteById() helper that
     deletes metadata FIRST, then vault. A throw on metadata.delete leaves
     the entry fully intact and readable; a throw on vault.deleteSecret
     reinstates a pending orphan marker (with orphanReason
     "vault-delete-failed") so readers see absence and operators can
     reconcile, AND surfaces a wrapped error whose message distinguishes
     "orphan marker persisted" from "orphan marker also failed to persist".
   - deleteById skips rows whose metadata is pending: true. This closes
     both the orphan-overwrite path (delete on an existing orphan marker
     used to nuke the vault entry it was tracking) and the concurrent
     put(new)+delete race (delete during the pending window of a new-entry
     put would wipe the vault entry mid-write).
   - get()/has() wrap expiry self-eviction in try/catch so a transient
     KV/vault failure during eviction does not surface as a thrown read.
   - deleteAll() collects per-row failures and throws AggregateError so a
     single bad row no longer silently masks deletion of the rest.

3. Orphan reason discriminator.
   CredentialMetadataRow gains an optional orphanReason field
   ("vault-write-failed" | "metadata-commit-failed" | "vault-delete-failed")
   and a corresponding exported OrphanReason type. Both put() orphan
   paths and the new delete() orphan path stamp the reason so operator
   tooling can pick a remediation strategy without re-running the original
   write. Legacy rows without the discriminator stay invisible to readers
   (covered by an explicit test).

14 new tests added; existing delete-removes-both test upgraded to assert
metadata-first ordering via spy call-order; existing put orphan tests
upgraded to assert orphanReason.

Supersedes #541.

https://claude.ai/code/session_01Wws8oZpB5imjKL2e7DRXtc

* fix(storage): restore ICredentialStore contract on ServerCredentialStore (sec)

Follow-up to #543. Two contract regressions discovered in code review:

H1: deleteAll() silently skipped pending/orphan rows because it now
    delegates to deleteById (added in #543) which short-circuits when
    pending=true. Result: no public API cleared orphan markers; a
    "wipe scope" call silently leaked the very rows operators need
    to clean up. Adds forceDeleteById that bypasses the pending-skip;
    deleteAll uses it. deleteById (single-key) unchanged.

H2: get/has/delete now threw TypeError for invalid keys because they
    all routed through vaultId(). ICredentialStore documents these as
    returning undefined/false on missing keys; throwing breaks
    substitutability with InMemoryCredentialStore / EncryptedKvCredentialStore.
    Adds isSafeKey() predicate; readers short-circuit; put() still
    throws via vaultId (only path that persists a colliding id).

M1: SAFE_SEGMENT widened from [A-Za-z0-9_-] to [A-Za-z0-9._:-] so
    natural credential names like "openai.prod" and "scope:billing"
    are accepted. Still rejects path separators and whitespace.

5 new tests + 1 rewritten test. Existing tests pass.

* docs(storage): clarify deleteById JSDoc + fix deleteByid typo (review)

Copilot review feedback on #544:

- deleteById's JSDoc claimed both delete() and deleteAll() share its
  code path, but deleteAll() deliberately routes through
  forceDeleteById() to bypass the pending-row short-circuit and
  reach sticky orphan markers. Updated to call out the asymmetry
  with a forward reference to forceDeleteById.
- Typo: 'deleteByid' -> 'deleteById' so the @link symbol resolves.

* refactor(storage): extract commitDelete to dedupe ServerCredentialStore delete paths

`deleteById` and `forceDeleteById` had nearly-identical bodies after the
pending-row check — the metadata-first ordering, orphan-marker rewrite,
and wrapped-error throw were duplicated, so a fix in one could silently
drift from the other. Lift the shared tail into `commitDelete(id, key,
existing, op)` so both callers route through one source of truth and
the only meaningful divergence (pending-row policy) stays at the call
site. Also corrects the stale `vaultId` JSDoc that still listed the
reader methods as callers — they were moved to `isSafeKey` in the same
PR for the ICredentialStore-contract substitutability fix.

https://claude.ai/code/session_011KMd9sERp2rguyekAi8a3u

---------

Co-authored-by: Claude <noreply@anthropic.com>